-
Notifications
You must be signed in to change notification settings - Fork 695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds true color support #1628
Adds true color support #1628
Conversation
I'm loving your work on this, thanks. I think for |
I'm using this to render simple canvases as a window. I made a modification for a custom constructor for TrueColorAttribute to allow providing the linux 256 color (calculated on a GPU) to avoid calculating it on the CPU, for flexibility. I'm taking quite a GC beating over the high frequency allocations of TrueColorAttribute being a reference type when I update the frame (it's video). The best solution I came up with was making the base Attributes Foreground, Background, and Value into read/write, but requires I run a modified library. public TrueColorAttribute(TrueColor foreground, Color foreground_color4, TrueColor background, Color background_color4) |
I looked into the ncurses library. With the current implementation, we need some sort of mapping to these indexed colors and keep track of them. Currently this sounds very inefficient to me. So I will not do this. The true collor support works just fine. The NetDriver also works on Linux and Mac. In my experience, the bottleneck for rendering various things is in the drivers using StringBuilder The whole Drivers could be optimized further, but this should be another task. |
This PR is pretty darn exciting. I'd love to get it merged as soon as it is ready. @veeman could you update us on your ability/plans to complete this awesome work? Thanks! |
Awesome, I still have to keep my promise to make HACC support Spectre.Console and it supports true color so this will be hot as snot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a big deal. And great.
- I believe this is a breaking change and that there's probably no way to NOT make it a breaking change. Thus this PR will likely define Terminal.Gui v2.x.
- The ColorPicker and other Scenarios that demo colors should be updated to use TrueColor support. I suggest that be done as separate PR's that are dependent on this one.
- I've asked for some changes throughout. I did not run this code and my review was fairly superficial. We'll need many more eyes on this given the magnitude of the changes.
Terminal.Gui/Core/ConsoleDriver.cs
Outdated
@@ -82,6 +82,86 @@ public enum Color { | |||
White | |||
} | |||
|
|||
/// <summary> | |||
/// 24bit color. Support translate it to 4bit(Windows) and 8bit(Linux) color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change summary to:
/// Represents a 24bit color. Supports translating to 4bit (Windows) and 8bit (Linux) colors.
/// <param name="red"></param> | ||
/// <param name="green"></param> | ||
/// <param name="blue"></param> | ||
public TrueColor (int red, int green, int blue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply correct source code formatting to all files in this PR (hit Ctrl-K-D in VS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto format applied.
/// <summary> | ||
/// Defines a true color attribute. | ||
/// </summary> | ||
public class TrueColorAttribute : Attribute { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused as to why this class is needed? Why not just put this into Attribute
and make it so true color is the default/assumed, supporting non-truecolor as a fallback?
When would someone use this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As describe before we need to decide which color escape sequence we should use. To keep backward compatibility i just introduced a new TrueColorAttribute class which holds the rgb values, like in the other PR.
This method has also the advantage that you only need to create new color escape sequences when the instance reference changes. However i can not evaluate if this has further side effects.
Furthermore there is no easy way to merge basic color and truecolor attribute class because any platform defines its own basic color range. The later must be unified first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to write a mapping algorithm for true color to Console Color (and vice versa)?
An approximation would be fine, I'm guessing someone has already done this elsewhere.
If we could translate between the two then we could just add the properties to Attribute
and add a new true color constructor. So the pseudocode would be something like:
public class Attribute
{
public int RedForeground { get; }
public int GreenForeground { get; }
public int BlueForeground { get; }
public int RedBackground { get; }
public int GreenBackground { get; }
public int BlueBackground { get; }
public Color Foreground { get; }
public Color Background { get; }
public Attribute (Color foreground, Color background)
{
Foreground = foreground;
Background = background;
var m = MapToTrueColor (foreground);
RedForeground = m [0];
GreenForeground = m [1];
BlueForeground = m [2];
m = MapToTrueColor(background);
RedBackground = m[0];
GreenBackground = m[1];
BlueBackground = m[2];
}
public Attribute (int redForeground, int greenForeground, int blueForeground, int redBackground, int greenBackground, int blueBackground)
{
RedForeground = redForeground;
GreenForeground = greenForeground;
BlueForeground = blueForeground;
RedBackground = redBackground;
GreenBackground = greenBackground;
BlueBackground = blueBackground;
Foreground = MapToBasicColor (redForeground,greenForeground,blueForeground);
Background = MapToBasicColor (redBackground,greenBackground,blueBackground);
}
[...]
}
That would allow clients to mix and match true color whenever they wanted. And even have an app that supports true color but falls back on basic colors when deployed to an older machine (with zero effort on the API consumers behalf)?
EDIT: Just realized Attribute
describes both foreground and background so have updated the pseudocode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, something like that could work. Maybe we can track the index by the lifetime of the Attribute class and use a database approach to select a free index by incrementing a static next-index value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this is the RGB breakdown. I might pull your branch and see if a 'transparent' and backwards compatible approach can be adopted following this approach.
Name | R | G | B |
---|---|---|---|
Black | 00 | 00 | 00 |
DarkBlue | 00 | 00 | 80 |
DarkGreen | 00 | 80 | 00 |
DarkCyan | 00 | 80 | 80 |
DarkRed | 80 | 00 | 00 |
DarkMagenta | 80 | 00 | 80 |
DarkYellow | 80 | 80 | 00 |
DarkGray | 80 | 80 | 80 |
Blue | 00 | 00 | FF |
Green | 00 | FF | 00 |
Cyan | 00 | FF | FF |
Red | FF | 00 | 00 |
Magenta | FF | 00 | FF |
Yellow | FF | FF | 00 |
Gray | C0 | C0 | C0 |
White | FF | FF | FF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veeman I've started working on this approach in https://github.com/tznind/gui.cs/tree/true-color
Once its ready (and assuming its viable) I will open a PR to your branch.
@@ -246,109 +370,109 @@ Attribute SetAttribute (Attribute attribute, [CallerMemberName] string callerMem | |||
case "TopLevel": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole switch statement scares me. So much brute-force code! It makes me wonder if there's a way of re-factoring it as part of this PR to use reflection or something to radically simplify it?
/// <summary> | ||
/// Determinates if the current console driver supports TrueColor output- | ||
/// </summary> | ||
public virtual bool SupportsTrueColorOutput { get => false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused as to why there's ALSO a method in CursesDriver.cs
called CanColorTermTrueColor
. Why not just have CursesDriver
implement an override of this method?
I think this method should be named SupportsTrueColor
for tersness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be abstract in the ConsoleDriver.cs
to force to be implemented in all drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CursesDriver
currently does not implement the TrueCollorAttribute feature.
On *unix, there is no easy way to determinate if the current terminal supports color output.
Despite ncruses provides a check for color support, it tells only if the library is compiled with color output support.
The unofficial community way is to check the environment variable COLORTERM
for some magic values. This does the static CursesDriver.CanColorTermTrueColor
function. This variable is also set under WSL, so i just retrive this information in the NetDriver
.
The virtual property SupportsTrueColorOutput
tells if the used driver and the used terminal environment supports color output.
The property UseTrueColor
is used to decide which mode should be used. If SupportsTrueColorOutput
is set then UseTrueColor
is also set, but can be uesed to enter baisc color rendering.
Marked for the v2.0 milestone. |
Using the PR "Adds true color support gui-cs#1628" from gui-cs/Terminal.Gui (https://github.com/gui-cs/Terminal.Gui/pull/1628/files), I added true color support to the Terminal.
Great work thanks. Do you have any idea how to achieve that? |
@veeman I've created a PR here veeman#2 with some improvements and changes. Including this new Scenario for opening images: Note: You can see that when toggling true/non-true color the exact RGB values rendered on screen change. I need to dig into why this is but I think its because each console will render |
We need to retarget this PR at the |
Fixes #48
Added true color support.
Based on the #103 pull request implementation.
As in this implantation the
struct Attribute
changed toclass Attribute
.Also there are now
TrueColorAttribute
andTrueColor
definitions wich can be used as dropin classes to provide the new feature.The abstract
ConsoleDriver
has two more properties which determinates the true color support state of the driver:The drivers
WindowsDriver
andNetDriver
are extended to render the new true colors.Also the
WindowsDriver
is optimized to do the rendering faster.The
CursesDriver
is not touched yet and would run only in the basic color mode.Moreover i added an exampel scenario and some basic unit test for this feature.
Currently only tested on Windows 10.
Any feedback is appreciated.